Skip to content

Fix it to emit a single diagnostic when both open and close quote are missing #1703

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 31, 2023
Merged

Fix it to emit a single diagnostic when both open and close quote are missing #1703

merged 1 commit into from
May 31, 2023

Conversation

TTOzzi
Copy link
Member

@TTOzzi TTOzzi commented May 25, 2023

Resolve #1691

When a string segment is located between missing quotes on both sides, looking ahead is interrupted, and each quote is diagnosed separately.
I have modified the logic to continue looking ahead even if a standalone string segment is encountered during the search.
Furthermore, I added a condition to the parentContextClause in order to modify the diagnostic, but I am uncertain if it is the appropriate approach.
If there is a better solution available, please let me know. Thank you.

@TTOzzi TTOzzi requested a review from ahoppen as a code owner May 25, 2023 15:18
TTOzzi
TTOzzi commented May 25, 2023
View reviewed changes
ahoppen
ahoppen reviewed May 25, 2023
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I would approach this is to add

public override func visit(_ node: StringLiteralExprSyntax) -> SyntaxVisitorContinueKind {

to ParseDiagnosticsGenerator. If both node.openQuote and node.closeQuote are missing, you can generate a custom error message.

@TTOzzi
Copy link
Member Author

TTOzzi commented May 25, 2023

The way I would approach this is to add

public override func visit(_ node: StringLiteralExprSyntax) -> SyntaxVisitorContinueKind {

to ParseDiagnosticsGenerator. If both node.openQuote and node.closeQuote are missing, you can generate a custom error message.

Ah, I see! You're referring to calling addDiagnostic there.
I will make changes. Thank you again for your kind feedback every time.

@TTOzzi TTOzzi force-pushed the fix-emit-single-diag-when-both-open-close-quote-missing branch from 6e31834 to 8e56b23 Compare May 25, 2023 17:47
@TTOzzi TTOzzi requested a review from ahoppen May 25, 2023 17:48
@TTOzzi TTOzzi force-pushed the fix-emit-single-diag-when-both-open-close-quote-missing branch from 8e56b23 to e65a90e Compare May 25, 2023 17:57
ahoppen
ahoppen reviewed May 25, 2023
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Looks very good. I only have a few minor comments to make it slightly more general

Tests/SwiftParserTest/AttributeTests.swift Outdated
fixIts: [#"insert '"'"#]
),
message: #"expected 'abc' to be surrounded by '"'"#,
fixIts: [#"insert '""'"#]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that this will change to insert '"' and '"' after #1651 is merged.

Copy link
Member Author

@TTOzzi TTOzzi May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for letting me know! Once that PR is merged, I will rebase to apply the changes 🙂

kimdv
kimdv reviewed May 26, 2023
View reviewed changes
@TTOzzi TTOzzi force-pushed the fix-emit-single-diag-when-both-open-close-quote-missing branch from e65a90e to 8847db8 Compare May 27, 2023 04:51
ahoppen
ahoppen approved these changes May 30, 2023
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. This looks great now. Can you rebase it now that #1651 is merged? Afterwards, I’ll trigger CI.

@TTOzzi TTOzzi force-pushed the fix-emit-single-diag-when-both-open-close-quote-missing branch from 8847db8 to 2a455d0 Compare May 30, 2023 22:56
@TTOzzi
Copy link
Member Author

TTOzzi commented May 30, 2023

Thank you. This looks great now. Can you rebase it now that #1651 is merged? Afterwards, I’ll trigger CI.

I'm done! Thank you @ahoppen 🙌

@kimdv
Copy link
Contributor

kimdv commented May 31, 2023

@swift-ci please test

@kimdv
Copy link
Contributor

kimdv commented May 31, 2023

@swift-ci please test windows

@ahoppen ahoppen merged commit 873cbce into swiftlang:main May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahoppen ahoppen ahoppen approved these changes

@kimdv kimdv kimdv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Parser should emit a single diagnostic if both open and close quote are missing from string literal
3 participants
@TTOzzi @kimdv @ahoppen